Skip to content

Add search box to CourseEnrollmentAllowed admin interface. - #16166

Merged
smarnach merged 1 commit into
openedx:masterfrom
open-craft:smarnach/filter-course-enrollment-allowed
Dec 11, 2017
Merged

Add search box to CourseEnrollmentAllowed admin interface.#16166
smarnach merged 1 commit into
openedx:masterfrom
open-craft:smarnach/filter-course-enrollment-allowed

Conversation

@smarnach

@smarnach smarnach commented Oct 4, 2017

Copy link
Copy Markdown
Contributor

This adds a search field to the CourseEnrollmentAllowed admin interface, and configures the list view to show individual fields instead of a string representation of the whole instance.

JIRA tickets: OSPR-1943

Discussions: None

Dependencies: None

Screenshots:

Without this change:
image

With this change applied:
image

Sandbox URL: https://pr16166.sandbox.opencraft.hosting/

Merge deadline: None

Testing instructions:

Navigate to https://pr16166.sandbox.opencraft.hosting/admin/student/courseenrollmentallowed/, log in as the standard staff user and try it.

Author notes and concerns: None

Reviewers

@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @smarnach! It looks like you're a member of a company that does contract work for edX. If you're doing this work as part of a paid contract with edX, you should talk to edX about who will review this pull request. If this work is not part of a paid contract with edX, then you should ensure that there is an OSPR issue to track this work in JIRA, so that we don't lose track of your pull request.

Create an OSPR issue for this pull request.

@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @smarnach! I've created OSPR-1943 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

Feel free to add as much of the following information to the ticket:

  • supporting documentation
  • edx-code email threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will still be done via the GitHub pull request interface. As a reminder, our process documentation is here.

@openedx-webhooks openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Oct 4, 2017
Comment thread common/djangoapps/student/admin.py Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: It seems like all tuple and list definitions in the other classes also include a comma at the end. Maybe better to do that here too, for consistency.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, changed that. (Some people prefer the trailing comma since it avoids a pitfall when reducing the length of the tuple to one. It's not done consistently in edx-platform, but seems to be done in this particular file.)

@smarnach
smarnach force-pushed the smarnach/filter-course-enrollment-allowed branch from 591b1da to 17194ae Compare October 4, 2017 18:31
@smarnach

smarnach commented Oct 4, 2017

Copy link
Copy Markdown
Contributor Author

@cgopalan The sandbox is now available – see the link in the PR description.

@cgopalan cgopalan left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

  • I tested this: Checked that the search field was visible and functional on the Student/Course Enrollment Allowed admin page.
  • I read through the code
  • I checked for accessibility issues
  • Includes documentation

@cgopalan

cgopalan commented Oct 4, 2017

Copy link
Copy Markdown

@smarnach seems like the jenkins/lettuce build failed. Not sure what the reason was. How do we trigger it again?

@smarnach

smarnach commented Oct 5, 2017

Copy link
Copy Markdown
Contributor Author

jenkins run lettuce

@smarnach

smarnach commented Oct 5, 2017

Copy link
Copy Markdown
Contributor Author

@cgopalan We can ask Jenkins to rerun the tests from the PR, but I'm not sure you would have permission.

@nedbat

nedbat commented Oct 23, 2017

Copy link
Copy Markdown
Contributor

@smarnach Sorry for the delay. This looks like a simple improvement. @mjfrey would this fall into your area of expertise?

@smarnach

Copy link
Copy Markdown
Contributor Author

@nedbat @mjfrey Could you please help me find a reviewer here? I think this change should be really uncontroversial and easy to review.

@mjfrey mjfrey left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

@smarnach

Copy link
Copy Markdown
Contributor Author

@mjfrey Thanks!

@smarnach
smarnach merged commit 02ddf4d into openedx:master Dec 11, 2017
@openedx-webhooks

Copy link
Copy Markdown

@smarnach 🎉 Your pull request was merged!

Please take a moment to answer a two question survey so we can improve your experience in the future.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production on Tuesday, December 12, 2017.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

@bradenmacdonald
bradenmacdonald deleted the smarnach/filter-course-enrollment-allowed branch May 18, 2019 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants